Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat(sea-orm-cli): output log about generated file name. #735

Merged
merged 7 commits into from
Jun 12, 2022
Merged

feat(sea-orm-cli): output log about generated file name. #735

merged 7 commits into from
Jun 12, 2022
+91 −1

Conversation

kyoto7250
Copy link
Contributor

@kyoto7250 kyoto7250 commented May 16, 2022 •
edited
Loading

PR Info

Adds

  • Add logging during code generation for sea-orm-cli.

Breaking Changes

  • No Breaking Changes

example output

❯ ../sea-orm/sea-orm-cli/target/debug/sea-orm-cli generate entity --database-url mysql://root@localhost/world --output-dir out/
Generating city.rs
    > Column `ID`: i32, auto_increment, not_null
    > Column `Name`: String, not_null
    > Column `CountryCode`: String, not_null
    > Column `District`: String, not_null
    > Column `Population`: i32, not_null
    > Column `created_at`: DateTimeUtc, not_null
Generating country.rs
    > Column `Code`: String, not_null
    > Column `Name`: String, not_null
    > Column `Continent`: Continent, not_null
    > Column `Region`: String, not_null
    > Column `SurfaceArea`: Decimal, not_null
    > Column `IndepYear`: Option<i16>
    > Column `Population`: i32, not_null
    > Column `LifeExpectancy`: Option<Decimal>
    > Column `GNP`: Option<Decimal>
    > Column `GNPOld`: Option<Decimal>
    > Column `LocalName`: String, not_null
    > Column `GovernmentForm`: String, not_null
    > Column `HeadOfState`: Option<String>
    > Column `Capital`: Option<i32>

It is my first PR in this repo.
Thank you in advance :)

billy1624
billy1624 reviewed May 16, 2022
View reviewed changes
Copy link
Member

@billy1624 billy1624 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @kyoto7250, thanks for the contributions! Looking good!

@kyoto7250 kyoto7250 marked this pull request as ready for review May 16, 2022 08:31
@kyoto7250
Copy link
Contributor Author

@billy1624
Could I request another review?

@kyoto7250
Copy link
Contributor Author

kyoto7250 commented May 16, 2022 •
edited
Loading

I missed CI failed notification.
I will check it.

The problem seems to be that tracing is not initialized during unit testing.

kyoto7250 and others added 2 commits May 17, 2022 13:55
@kyoto7250 @billy1624
Update sea-orm-cli/src/commands.rs
ac586f7 
Avoid multiple initializations

Co-authored-by: Billy Chan <ccw.billy.123@gmail.com>
@kyoto7250
show info per column
cbf0fa3
@kyoto7250
Copy link
Contributor Author

I fixed it.
Could you review it again?

@kyoto7250 kyoto7250 changed the title feat(sea-orm-cli): output lof about generated file name. feat(sea-orm-cli): output log about generated file name. May 17, 2022
@kyoto7250 kyoto7250 requested a review from billy1624 May 18, 2022 00:21
billy1624
billy1624 reviewed May 18, 2022
View reviewed changes
Copy link
Member

@billy1624 billy1624 left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @kyoto7250, sorry for being so picky. We want to keep the code as clean as possible. I still think there is room for refactoring, please review the commit below and feel free to cherry pick it

@kyoto7250
Copy link
Contributor Author

kyoto7250 commented May 18, 2022 •
edited
Loading

@billy1624
Of course. I'll fix it.
Thank you for giving me a better way to write!

#735 (review)

@kyoto7250 kyoto7250 requested a review from billy1624 May 18, 2022 07:13
billy1624
billy1624 approved these changes May 18, 2022
View reviewed changes
Copy link
Member

@billy1624 billy1624 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! @kyoto7250

@billy1624 billy1624 requested a review from tyt2y3 May 18, 2022 09:06
@lcptrs
Copy link

lcptrs commented Jun 7, 2022

Hey there, I’m still quite new to rust in general so more of question here.
When would you use std::fmt::Display trait instead? Or in other terms, why not to use it here?

@billy1624
Copy link
Member

Hey @Irykson, welcome and good questions! I think generally speaking std::fmt::Display is for printing debug info and shows all the attribute inside a data structure. But here, we're printing column info with in custom format, hence, I don't think it's ideal to implement std::fmt::Display for this purpose.

@kyoto7250
Copy link
Contributor Author

I aggre @billy1624 .

@lcptrs
Copy link

lcptrs commented Jun 7, 2022

Thanks for the reply and your warm welcome @billy1624!
I really don't want to hijack this PR to start a discussion or a Q&A, so feel free to shut it here :)
I just found this guideline which says, Display is intended for user-facing output which was also my understanding of this trait. So I actually thought, the reasoning behind not using it here, has more to do with the fact, that get_info could be thought of a somewhat niche case and you want another default output for Display in the future.

@kyoto7250
Copy link
Contributor Author

I just found this guideline which says, Display is intended for user-facing output which was also my understanding of this trait. So I actually thought, the reasoning behind not using it here, has more to do with the fact, that get_info could be thought of a somewhat niche case and you want another default output for Display in the future.

maybe, yes :)

and I think it is not now.

@tyt2y3
Copy link
Member

tyt2y3 commented Jun 12, 2022

@Irykson yeah usually what would happen is when we impl Display get_info will be called inside the function

@tyt2y3 tyt2y3 merged commit 0ce8ee6 into SeaQL:master Jun 12, 2022
@tyt2y3
Copy link
Member

tyt2y3 commented Jun 12, 2022

Thank you all for making a better developer experience for all

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@billy1624 billy1624 billy1624 approved these changes

@tyt2y3 tyt2y3 Awaiting requested review from tyt2y3

Assignees
No one assigned
Labels
None yet
Projects
SeaQL Dev Tracker
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[CLI] Show progress when generating entity files
4 participants
@kyoto7250 @lcptrs @billy1624 @tyt2y3